Use nvcomp defaults for algo options. #450
Conversation
bdice
added
improvement
|
@madsbk @Alexey-Kamenev @thomcom could you please look over these changes when you have a moment? 🙂 |
|
As it appears we do tests constructing compressors with default arguments, this should already be covered in the existing test suite kvikio/python/kvikio/tests/test_nvcomp.py Lines 195 to 217 in 3ce8bcc Though please let me know if I've missed anything here |
|
|
||
| def _get_comp_temp_size( | ||
| self, | ||
| size_t batch_size, | ||
| size_t max_uncompressed_chunk_bytes, | ||
| ) -> (nvcompStatus_t, size_t): | ||
| ) -> tuple[nvcompStatus_t, size_t]: |
There was a problem hiding this comment.
hmm, how did this ever work? I guess cython is less strict with parsing.
There was a problem hiding this comment.
Both are valid, the type format we had before is called a ctuple and has been around in Cython longer
Support for typing.Tuple is relatively new
There was a problem hiding this comment.
Cython already checks whether the value is an int or None based on the function signature. So the exceptions can be dropped. If we wanted to be extra careful, we could add an assert. Though likely that is unneeded
Example of this behavior demonstrated below:
In [1]: %load_ext cython
In [2]: %%cython
...:
...: from typing import Optional
...:
...: class Class:
...: def __init__(self, v: Optional[int] = None):
...: print(f"v = {v}")
...: self.v = v
...:
In [3]: Class()
v = None
Out[3]: <_cython_magic_96fdfa8e6c04d6dbb7c0c7398f039f6decfcbe5b.Class at 0x10524ad10>
In [4]: Class(1)
v = 1
Out[4]: <_cython_magic_96fdfa8e6c04d6dbb7c0c7398f039f6decfcbe5b.Class at 0x104f4c610>
In [5]: Class(3.14)
---------------------------------------------------------------------------
TypeError Traceback (most recent call last)
Cell In[5], line 1
----> 1 Class(3.14)
TypeError: Argument 'v' has incorrect type (expected int, got float)
Co-authored-by: jakirkham <[email protected]>
|
Thanks everyone for the reviews! I'll wait another work day to make sure folks are happy with the way the comments like #450 (comment) have been resolved. |
|
/merge |
Update the nvCOMP version used for compression/decompression to 4.0.1. See also: rapidsai/rapids-cmake#633 rapidsai/cudf#16076 Depends on #450. Authors: - Vukasin Milovanovic (https://github.com/vuule) - Bradley Dice (https://github.com/bdice) Approvers: - Mads R. B. Kristensen (https://github.com/madsbk) - Bradley Dice (https://github.com/bdice) URL: #449
This PR updates the nvcomp bindings to use nvcomp's defaults rather than hardcode the default options. The defaults changed in nvcomp 4.0.1, so this is needed for #449.